I don't know if this is necessary. The BadZipFile message looks reasonable to me.

@DiddiLeija Can you elaborate on your remark? As explained, the message is completely unhelpful to me because I don't even know what file or wheel it is about. It might be useful to the expert who knows the precise internals of what pip install does under the hood, but I'd expect it to come across as rather cryptic to anybody else.

(note also that the stacktrace is an excerpt; I have no reason to assume that the wrapt installation has anything to do with it)

You're right, my misreading:

Since the information which file caused the error is missing, I don't have any means to further narrow down this issue (e.g. check which files are affected, compare checksums, etc). This change should help me and anyone else facing such issues.

Thinking a bit about this, this could be a good aproach, but we may have to sketch how the message must look like.

BTW, before going ahead, I recommend you to write a news entry, because this change is not trivial (I think). See https://pip.pypa.io/en/stable/development/contributing/#news-entries for more information. Also, you may have to write a test for this behavior.

Instead of raising RuntimeError, this should raise either UnsupportedWheel or a new exception subclassing InstallationError (e.g. InvalidWheel). InstallationError is a misnomer (legacy reasons), it basically fails the process with a nice error message instead of a bunch of traceback (the traceback is only shown in verbose mode).

I think it's also likely better to handle this in Distribution.from_wheel() instead and add a docstring describing the method should not raise BadZipError and should raise the new exception instead on an invalid wheel. A test that creates and tries to pip install an invalid zip file is needed as well.

#9964 is one example of what situations might cause this corruption.

Small addition: The error occurred with pip==21.2.4, and in the meanwhile, 135faab has introduced the abstraction layer of MemoryWheel / FileSystemWheel.

With that in mind, note how my stracktrace is slightly different as it would be now. It goes

• get_wheel_distribution
• Distribution.from_wheel
• ZipFile.__init__

but now it would instead go

• get_wheel_distribution
• Distribution.from_wheel
• FileSystemWheel.as_zipfile
• ZipFile.__init__.

@DiddiLeija any pointers for which kind of test I should write? A unit test in unit.test_metadata which passes a mocked wheel which manually raises BadZipFile, or a full-on functional test where I try to load a corrupt wheel file, or something inbetween?

I'm not too familiar with pip's test concept.

I think a functional test would be better in this case.

@uranusjr I followed your instructions. Also, I added a news file as suggested. A test is still missing.

I'm not sure whether InvalidWheel or CorruptWheel is preferable for an exception name.

Note that in the case of HTTP transfer, i.e. a call of dist_from_wheel_url, the wheel.location attribute will amount to the name of the temporary file created by LazyZipOverHTTP. The new message Wheel foo located at /tmp/blah won't be an improvement in this case, but it's probably not worse than an unhandled BadZipFile (and corruptions aren't likely to occur over HTTP, that's what the transport layer is for). But that's not the use case we want to improve here, anyway.

@DiddiLeija gotcha.

Let's go with InvalidWheel, we can always change it if needed; pip doesn't have a public Python API so renaming things is not a big deal. Adding a new class means it'll be easier to do something to the lazy zip case if we want to, so let's get this in and think about how that can be improved afterwards.

Following functional.test_install_from_broken_wheel to run pip install $corrupt_file.whl, it's not quite clear to me how I should test for InvalidWheel to be thrown.

I see two options:

1. somehow grepping through stderr
2. parametrizing test_install_from_broken_wheel to feed it a wheel broken on the zip level as well. In both cases (BadZipFile vs. in whatever way brokenwheel-1.0-py2.py3-none-any.whl is corrupt) the observable result should be “stacktrace && installation unsuccessful”.

Writing a unit test for this behavior proved a lot simpler, so it sounds reasonable to me to go for option 2 and leave the unit test in, so we can test both the general behavior (“some stacktrace”) and the specific handling (InvalidWheel).

I think option 2 makes more sense as well.

Done, and in addition

• Incorporated black reformatting
• verified that my unit test failed when undoing the change.

Sorry for all the force-pushes, I know it requires reapproval. I forgot to squash some stuff, I'm finished now.

pre-commit does not like trailing spaces.

pre-commit does not like trailing spaces.

Ah, I forgot to pre-commit install so I didn't notice that in this session. Does not complain locally, should be green now.

So sorry for the broken commit message in 6d0612d, fixed by add2ab0. Happened due to a reset so I could run pre-commit again.

Just checking in, is any further work required from my side?

Porbably not (at least not from me). I'll leave this open in case someone wants to take a look, but also add this to 22.0 so it can be merged if no-one has any objections.

Thanks a lot for this @lukasjuhrich! ^>^